Skip to content

[fix] Kinetic devel mongocxx c++11 #224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

bbferka
Copy link
Contributor

@bbferka bbferka commented May 25, 2018

@furushchev @hawesie it seems like #223 did not completely solve the problem. I added ros-shadow-fixed to my sources and installed libmongocxx but I still end up with the undefined behaviour. This is weird because building from source workes just fine, so I've been looking through the Jenkins build logs on build.ros.org:

http://build.ros.org/job/Kbin_uX64__libmongocxx_ros__ubuntu_xenial_amd64__binary/14/consoleFull

It looks like it built without C++11 support (search for c++11 in the log) on the build farm. The reason is that during this build there is no ROS environment being sourced, so the check for ROS_DISTRO does not work.

I've changed this now to check if the variable is set, and if it's not check for the ubuntu version; What do you think?

@hawesie
Copy link
Member

hawesie commented May 29, 2018

lsb_release is not available on all platforms where this might be built, but I think this extra check is really only needed for the build farms which I assume will all have it. Building from source in a normal catkin ws shouldn't reach that part.

@hawesie hawesie merged commit 178246f into strands-project:kinetic-devel May 29, 2018
@bbferka
Copy link
Contributor Author

bbferka commented May 29, 2018

@hawesie yes, I was afraid that lsb_release could be a problem on other platforms. If it does not exist the command will still run, and will simply build without c++11 support. Thanks for merging. I hope this fixes our error now.

EDIT: apparently they don't have lsb_release:

http://build.ros.org/job/Kbin_uX64__libmongocxx_ros__ubuntu_xenial_amd64__binary/15/consoleFull

I again built without C++11; Do you know if there is a simple way of testing this without sending PRs and releasing?

@hawesie
Copy link
Member

hawesie commented May 29, 2018

Oh no! I don't know I'm afraid. Paging @marc-hanheide who might know a bit more about testing builds for the build farm. (he's away for a few days though)

@marc-hanheide
Copy link
Member

Did you check the development builds on our own build farm: https://lcas.lincoln.ac.uk/buildfarm/job/Kdev__mongodb_store__ubuntu_xenial_amd64/

First check is to see if they build the way expect them.

@bbferka
Copy link
Contributor Author

bbferka commented May 29, 2018

@marc-hanheide yes, those builds are correct. It is interesting, that even on the ROS buildfarm, when looking at:

http://build.ros.org/job/Kbin_uX64__mongodb_store__ubuntu_xenial_amd64__binary/16/consoleFull

, it builds with the correct flag, finds that it is kinetic, uses the c++11 flag;

It is only when building the deb for libmongocxx, that it fails at every check and ends up building without C++11 support.

@marc-hanheide
Copy link
Member

Yeah, the debs only build with a bare minimum of dependencies, i.e. likely without lsb_release installed.

@bbferka
Copy link
Contributor Author

bbferka commented May 29, 2018

hmm, so do you see any other CMake magic options for building without C++11 on indigo or could we create a separate indigo-devel branch where build is without c++11 and the kinetic-devel branch has it turned on.

@furushchev
Copy link
Collaborator

I'm grasping at a straw, how about checking /etc/lsb-release file manually in CMakeLists.txt then?

@bbferka
Copy link
Contributor Author

bbferka commented May 29, 2018

@furushchev good point; I will try with that; Although it would be really nice to be able to see the behaviour without the need for a new release :).

@marc-hanheide yes it seems to be with really the bare minimum. What struck me odd is that libmongocxx_ros is a catkin package, but the deb is built without a ROS environment sourced (otherwise ROS_DISTRO would be defined)

@bbferka
Copy link
Contributor Author

bbferka commented May 29, 2018

@furushchev after looking at this for a while, it seems like /etc/os-release is the recommended file to look at. One problem though: the content of it is not well defined, i.e.: Ubuntu has VERSION_CODENAME= but Debian does not :(. I am fine with looking only at Ubuntu releases, although this is not an elegant solution.

Different idea that a colleague just gave me. What about putting lsb-release as a buildtool-dependency? This way it would get installed in the docker on the build farm.
@hawesie You wrote that lsb_release is not available for all platforms that the build is performed for. Which platforms do you think would don't have it ? We only looked at Denian and Ubuntu, and those looked fine.

@hawesie
Copy link
Member

hawesie commented May 29, 2018

@bbferka I build on OSX, but in a catkin environment so the distro version is available.

@bbferka
Copy link
Contributor Author

bbferka commented May 29, 2018

@hawesie true, in that case putting lsb-release in the package.xml is not a good idea. I presume it would break building from source on OSX. I will open a PR with the suggestion from Yuki to read /etc/os-release from CMake and lookup the codename from there. I really hope that solves the build on the ros farm.

@dirk-thomas
Copy link

The reason is that during this build there is no ROS environment being sourced, so the check for ROS_DISTRO does not work.

This is not correct. When looking through the build log you will find the following line:

if [ -f "/opt/ros/kinetic/setup.sh" ]; then . "/opt/ros/kinetic/setup.sh"; fi && \

So the environment is being set up (if the ROS package depends on catkin).

The binary jobs for the other packages have actually been built with c++11. The reason is that mongodb_log depends on roslib which depends on ros_environment which brings in the ROS_DISTRO environment variable. The same for mongodb_store: it depends on rostest which depends on roslaunch which depends on roslib which depends on ros_environment.

Only libmongocxx_ros didn't have that environment variable set and therefore built without c++11 becasue it doesn't have a (recursive) dependency on ros_environment. If you add this dependency to the package it will enable c++11 just fine.

@bbferka
Copy link
Contributor Author

bbferka commented Jun 5, 2018

@dirk-thomas thank you for your detailed explanation. This explains everything. I saw that all the other packages were built correctly, but I did not know about the souerce of ROS_DISTRO. I will definitely make the necessary changes and send a new PR, such that the release is up to the standards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants